Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Check the ambientness of a symbol name before attempting to trim it #26312

Merged
merged 2 commits into from
Aug 9, 2018

Conversation

weswigham
Copy link
Member

Fixes #26295

An export assigned namespace from a declaration file with an augmentation in another file is:

  1. Not a source file itself
  2. Has no value declaration
  3. Does not have an ambient module name for a symbol name
  4. Still looks like a module symbol as far as symbol flags go

And so naming the module it corresponds to proves to be a more difficult process than we had assumed! If we have a resolver, we can find the file corresponding to the first non-augmentation declaration, however when a resolver is not available we must fall back to the full path of the containing file of that declaration. We actually had a bunch of baselines which exhibited this bug in the types baseline (though none in declaration emit).

@weswigham weswigham requested a review from a user August 8, 2018 22:23
@weswigham
Copy link
Member Author

@typescript-bot test this just in case

@typescript-bot
Copy link
Collaborator

typescript-bot commented Aug 8, 2018

Heya @weswigham, I've started to run the extended test suite on this PR at 7ff7dcc. You can monitor the build here. It should now contribute to this PR's status checks.

@TejasQ
Copy link

TejasQ commented Aug 9, 2018

Thank you guys for your good, quick work here!

@@ -232,8 +232,17 @@ namespace ts.moduleSpecifiers {
}

function tryGetModuleNameFromAmbientModule(moduleSymbol: Symbol): string | undefined {
const decl = moduleSymbol.valueDeclaration;
if (isModuleDeclaration(decl) && isStringLiteral(decl.name)) {
const decl = forEach(moduleSymbol.declarations, d => {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems like this could use find and boolean operators.

@@ -638,6 +638,10 @@ namespace ts {
return false;
}

export function getNonAugmentationDeclaration(symbol: Symbol) {
return forEach(symbol.declarations, d => isExternalModuleAugmentation(d) ? undefined : d);
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could use find

@weswigham
Copy link
Member Author

@Andy-MS another quick look?

@weswigham weswigham merged commit fce3d9f into microsoft:master Aug 9, 2018
@weswigham weswigham deleted the verify-ambientness-of-name branch August 9, 2018 20:20
errendir added a commit to errendir/TypeScript that referenced this pull request Aug 11, 2018
* origin/master: (283 commits)
  Don't error on destructure of private property with computed property syntax (microsoft#26360)
  getDefaultExportInfo: Use `getImmediateAliasedSymbol` instead of `getAliasedSymbol` (microsoft#26364)
  review comments
  restore old algorithm
  Dont use baseURL relative absolute paths in declaration emit, use absolute paths in bundle emit (microsoft#26341)
  Update user baselines (microsoft#26358)
  Don't store @template constraint in a TypeParameterDeclaration node (microsoft#26283)
  fixAddMissingMember: Support interface and don't crash on type parameter (microsoft#25995)
  Don't include class getter in spread type (microsoft#26287)
  Don't crash on computed property in destructure (microsoft#26334)
  Check the ambientness of a symbol name before attempting to trim it (microsoft#26312)
  Still generate signatures in SkipContextSensitive mode just to match on return types (microsoft#25937)
  fix handling if there is no commonPrefix
  Actually add sorting of elaboration text to user baselines
  Ping ryan instead of mohammed for user PRs now
  handle failed lookups
  make it work for root directory
  really, really fix test(?)
  add test
  fix commonPrefix handling
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Erroneous specifiers in declaration files.
4 participants